Skip to content

fix(geocoding): Navigate button silently fails for search results and drag-moved pins (#173)#174

Merged
jasoneplumb merged 1 commit intomainlinefrom
fix/navigate-button-stale-pin
Apr 26, 2026
Merged

fix(geocoding): Navigate button silently fails for search results and drag-moved pins (#173)#174
jasoneplumb merged 1 commit intomainlinefrom
fix/navigate-button-stale-pin

Conversation

@jasoneplumb
Copy link
Copy Markdown
Owner

Summary

  • The "Navigate" button next to Copy in the geocode-bar tracked its destination via a pinLayer.on('layeradd') listener — fires only when a new layer is added to the group.
  • That misses two flows:
    • Search → Go to location: handler calls pinLayer.clearLayers() and never adds a new pin → currentPinLatLng stays null (silent fail) or stale (worse — navigates to a previous long-press pin).
    • Pin drag: the marker moves but isn't re-added → listener doesn't fire → Navigate goes to the original drop location, not the dragged one.

Fix

Thread the destination L.LatLng through showGeocodeBar(label, copyText, latlng). That function is the single setter all three flows (search, long-press, drag) already converge on, so capturing the destination there is reliable. Removed the unreliable layeradd listener.

Also replaced the silent-return guard with a "No destination set" toast so any future regression surfaces visibly instead of vanishing.

Closes #173.

Test plan

  • npm run type-check clean
  • npm run lint clean
  • npm test clean (633 tests)
  • Manual: search → Go to location → Navigate goes to that result
  • Manual: long-press → Navigate goes to the pin
  • Manual: long-press → drag pin → Navigate goes to the dragged location
  • Manual: tap Navigate before any destination is set → "No destination set" toast

🤖 Generated with Claude Code

The "Navigate" button on the geocode-bar tracked its destination via a
pinLayer 'layeradd' listener. That fires only when a new layer is added,
which misses two flows:

- Search "Go to location" calls pinLayer.clearLayers() and never adds
  a new pin, so currentPinLatLng stays null (silent fail) or stale
  (worse — navigates to the previous long-press pin).
- Pin drag moves the same marker without re-adding, so the listener
  doesn't fire and Navigate goes to the original drop location, not
  the new dragged one.

showGeocodeBar() is the single setter all three flows (search,
long-press, drag) already converge on, so threading the L.LatLng
through it captures the current destination reliably. Removed the
'layeradd' listener; replaced the silent-return guard with a toast
so any future regression surfaces visibly.

Closes #173

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jasoneplumb jasoneplumb added bug Something isn't working P1 Critical — fix immediately review-requested Triggers Claude code review labels Apr 26, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Code Review

Overview

This PR fixes a genuine bug: the layeradd listener was an unreliable way to track the Navigate button's destination because it only fires when a layer is added — missing both the search flow (pin cleared, not re-added) and the drag flow (marker moved in-place, not re-added). Threading latlng through showGeocodeBar is the right fix since that function is already the single convergence point for all three write paths.

Net: -19/+13 lines, simpler invariant, better error visibility. Approve-ready with minor notes.


Strengths

  • Root cause is correct. showGeocodeBar is exactly the right place to own currentPinLatLng — it is already called by all three flows (search result selection, long-press geocode success, long-press geocode fallback).
  • Removed complexity beats added complexity. Deleting the layeradd listener and its getLayers() inspection is a clear win; the new version has no indirect reads of Leaflet layer state.
  • Toast on null guard is a good regression surface. The previous silent return would have hidden any future missed call-site.
  • Type signature is consistent_showGeocodeBar type alias is updated alongside the implementation, so TypeScript will catch any call-site that wasn't updated.

Issues / Suggestions

1. Pin-drag path — verify showGeocodeBar is called with the dragged position (not the drop position)

The diff updates the two explicit showGeocodeBar call sites inside the reverse-geocoding callback, and the search-result handler. If pin-drag re-triggers reverse geocoding and routes through those same callbacks, the fix is complete. The PR description implies this is the case ("long-press → drag pin → Navigate goes to the dragged location"), but the drag wiring isn't visible in the diff. Worth a quick confirmation that the dragend handler eventually calls showGeocodeBar with marker.getLatLng() (or triggers reverse-geocoding which does).

2. L.latLng(lat, lng) construction in the search handler (line ~266)

_showGeocodeBar?.(label, copyText, L.latLng(lat, lng));

lat and lng are already available as numbers here, so this is fine. Just worth noting that this constructs a new L.LatLng from the parsed result coordinates rather than reusing a Leaflet object — that's correct because search results don't go through pinLayer, so there is no marker whose getLatLng() could be used instead.

3. No automated test coverage for the fixed flows

The CLAUDE.md notes tests are pure-function only (no DOM/Leaflet integration), so this is expected and acceptable. The manual test plan in the PR description covers all three fixed flows plus the new null-guard toast. No change needed, just flagging it as an acknowledged gap.

4. Minor: comment in addReverseGeocoding could be trimmed

// "Navigate" button on the geocode-bar — start guidance to the destination
// associated with the currently-shown bar. showGeocodeBar() is the single
// setter so search-results, long-press, and pin-drag all stay in sync.

The second and third lines are valuable context (they explain the why of the design). The comment is worth keeping as-is.


Summary

Area Assessment
Correctness ✅ Fix is sound for the three identified flows
Code quality ✅ Simpler, fewer moving parts
Conventions ✅ Matches project style
Type safety ✅ All call-sites updated
Tests ⚠️ No automated tests (expected per project constraints; manual plan covers it)
Security ✅ No concerns

The one thing worth confirming before merge is that the drag dragend path does reach showGeocodeBar with the updated position (point 1 above). If that's already verified through manual testing, this is good to merge.

@jasoneplumb
Copy link
Copy Markdown
Owner Author

Confirming the drag path: dropPin's pin.on('dragend') (geocoding.ts:813) calls reverseGeocode(pin.getLatLng()) after a 300 ms debounce. reverseGeocode(latlng) then calls showGeocodeBar(addr, addr, latlng) with that updated position. So the drag flow is covered by the existing reverse-geocode callback updates.

@jasoneplumb jasoneplumb merged commit e959ba2 into mainline Apr 26, 2026
4 checks passed
@jasoneplumb jasoneplumb deleted the fix/navigate-button-stale-pin branch April 26, 2026 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working P1 Critical — fix immediately review-requested Triggers Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: Navigate button silently fails for search results and drag-moved pins

1 participant